-
Notifications
You must be signed in to change notification settings - Fork 9
Support PHPUnit 9 #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool if we can drop the listener and printer classes!
composer.json
Outdated
"phpunit/phpunit": "^7.0 || ^8.0", | ||
"php-http/message": "^1.0", | ||
"guzzlehttp/psr7": "^1.0", | ||
"php": ">=7.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i prefer to express this as ^7.1 || ^8.0
- we don't know if there will be breaking changes in PHP 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the argument from Symfony, basically:
"Using
^7.1 || ^8.0
will stop people from test with PHP 9 and you (the maintainer) will be responsible for making sure that it works on PHP 9. Using>=7.1
allows other to try with PHP 9 and report/fix issues.
However, I dont really mind either way. I'll change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, i don't understand that reasoning. i prefer it when people do a pull request to allow php 9 and we have a controlled way of seeing what is going on. otherwise people will just start using it and not be aware that its maybe not supported, and just complain. i feel with the restriction, it is less on the maintainer to make sure it works on php 9 than it is when we already allow it...
thanks for the change.
They were never used by us.. I dont remember why we added them in the first place. We can see if people complain in 3.0 |
a lot of our builds fail, not ideal. but with this change, there is one more green build, so lets merge it. |
We should consider removing some jobs and make sure others passes. |
I think I've done this correctly.